Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addressing ECAL local reconstruction on GPU issues #33116

Merged
merged 26 commits into from
Mar 24, 2021

Conversation

thomreis
Copy link
Contributor

@thomreis thomreis commented Mar 9, 2021

PR description:

This PR addresses some issues that have been left open during the initial PR #31719 . They are tracked in issue #32480 .
The fixed issues:

  • Update the ECAL ESProducts and move them to a more correct place
  • Migrate GPU code to use common constants and functions
  • Some cleanup in code comments

PR validation:

Passes matrix WFs 10824.512 and 11634.512.
No changes in the RecHits output.

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-e8dd18/13573/summary.html
COMMIT: 0439265
CMSSW: CMSSW_11_3_X_2021-03-16-2300/slc7_amd64_gcc900
Additional Tests: GPU
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33116/13573/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2639881
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 2639852
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
  • Checked 155 log files, 37 edm output root files, 37 DQM output files
  • TriggerResults: no differences found

@jpata
Copy link
Contributor

jpata commented Mar 18, 2021

Thx for the updates. As far as I see, this is the pending question before the reco sig. I can also run a comparison myself, I was just wondering if it was already done. Thanks!

Did you check that there are no changes to the GPU outputs?

@thomreis
Copy link
Contributor Author

The additional GPU tests seem to consist of WFs 10824.502 and 10824.512 and they passed. However, it looks like the two WFs were not included in the comparisons (https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_11_3_X_2021-03-16-2300+e8dd18/41754/). Is that the normal behaviour?

@jpata
Copy link
Contributor

jpata commented Mar 18, 2021

There is no automatic comparison of GPU workflows at the moment, it needs to be implemented. I can run a check wrt. the baseline (or if you already did, let me know).

@thomreis
Copy link
Contributor Author

I am running a comparison with 10824.512 atm.

@thomreis
Copy link
Contributor Author

thomreis commented Mar 18, 2021

Comparing WF 10824.512 ECAL sorted RecHits from CMSSW_11_3_0_pre3 with the ones from CMSSW_11_3_0_pre3 + this PR on 100 events, I get the same number of RecHits with the same mean and std dev. values for all variables. For EB and EE. So this looks like there are no changes in the output, as expected.

@jpata
Copy link
Contributor

jpata commented Mar 19, 2021

+reconstruction

@thomreis
Copy link
Contributor Author

@cms-sw/db-l2 , @cms-sw/alca-l2 do you have questions or comments?

@jpata
Copy link
Contributor

jpata commented Mar 22, 2021

@cms-sw/db-l2 @cms-sw/alca-l2

@ggovi
Copy link
Contributor

ggovi commented Mar 22, 2021

+1

@silviodonato
Copy link
Contributor

kind reminder @cms-sw/alca-l2

@yuanchao
Copy link
Contributor

+1

  • PR updated to comply the coding rules
  • matrix tests and unit tests all passed, as well as the GPU addon tests.
  • static build warnings mainly come from the deprecated es<>.get() (none part of this PR)

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

@qliphy
Copy link
Contributor

qliphy commented Mar 24, 2021

+1

@cmsbuild cmsbuild merged commit c0f6283 into cms-sw:master Mar 24, 2021
@thomreis thomreis deleted the ecal-local-reco-gpu-fix-issues branch April 9, 2021 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants